Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Design plugin feature #289

Draft
wants to merge 15 commits into
base: master
Choose a base branch
from

Conversation

tayoogunbiyi
Copy link
Contributor

No description provided.

@csr
Copy link

csr commented Jun 24, 2020

Hi @juped & everyone, Eyitayo, Mwiza, and I have been working on a solution to decompose the howdoi codebase into howdoi.py, BasePlugin (base class for all plugins) and StackOverflowPlugin (a subclass of BasePlugin which is used as the default plugin for the user query if no plugins are specified).

We'd appreciate feedback on:

  • The general code architecture
  • Coding style and general improvements
  • Suggestions on how users will be able to install third-party plugins (we thought about having a system that fetches plugins from your GitHub account). In general though, users will be able to manually add plugin files in the plugins folder
  • Where the cache needs to be stored. Should it be a global variable that all plugins can access?
  • Everything else that comes to your mind :)


from cachelib import FileSystemCache, NullCache

from pyquery import PyQuery as pq

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this makes it easier to understand this.

Suggested change
from pyquery import PyQuery as pq
from pyquery import PyQuery as query_xml

hyperlinks = element.find('a')

for hyperlink in hyperlinks:
pquery_object = pq(hyperlink)

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Again, I think this is a more helpful variable name.

Suggested change
pquery_object = pq(hyperlink)
xml_extractor = query_xml(hyperlink)

for hyperlink in hyperlinks:
pquery_object = pq(hyperlink)
href = hyperlink.attrib['href']
copy = pquery_object.text()

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
copy = pquery_object.text()
copy = xml_extractor.text()

replacement = copy
else:
replacement = "[{0}]({1})".format(copy, href)
pquery_object.replace_with(replacement)

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
pquery_object.replace_with(replacement)
xml_extractor.replace_with(replacement)

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This feedback is great, we'd like to keep this PR a "refactor" and save these valuable renames for a later PR so that we don't change the previous code too much

replacement = "[{0}]({1})".format(copy, href)
pquery_object.replace_with(replacement)

def get_link_at_pos(self, links, position):

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This can be a static method.

except TypeError:
return element.text()

def _get_search_url(self, search_engine):

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
def _get_search_url(self, search_engine):
@staticmethod
def _get_search_url(search_engine):

return [a.attrib['href'] for a in html('.l')] or \
[a.attrib['href'] for a in html('.r')('a')]

def _extract_links_from_duckduckgo(self, html):

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This can also be a static method

Suggested change
def _extract_links_from_duckduckgo(self, html):
@staticmethod
def _extract_links_from_duckduckgo(html):

results.append(parsed_url[0])
return results

def _extract_links(self, html, search_engine):

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
def _extract_links(self, html, search_engine):
@staticmethod
def _extract_links(html, search_engine):

return filtered_proxies

def extract(self):
print("Hello extract")

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure why this is here.
If it's intended for it to be overriden in child classes, I'd suggest either raising a NotImplementedError or just leave it as it is (without the print statement).

'Safari/536.5'), )


def _random_int(width):

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Assuming that this just generates a random number, would it be easier to use random.randint?

}


class BasePlugin():

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Architecture thoughts:

import enum.Enum as Enum

class Source(Enum):
   CLI = 0
   DISCORD_MESSAGE = 1

class ServiceConnectionError(Exception):
  pass

class Plugin:
    name = "UnimplementedPlugin"
    def __init__():
      pass
    def resolve(self):
      pass
    def extract(self, source, data): # raises an exception if this fails
      pass

class PluginStack(list):
   def __init__(self, plugins):
     super(PluginStack, self).__init__(plugins)
   def handle_input(self, source, data):
     tries = [ ]
     for plugin in self:
       try:
         plugin.extract(source, data)
         plugin.resolve()
       except <any of the relevant exceptions>:
         tries.append("howdoi could not get a response from {}".format(plugin.name))

Further possible areas for improvement:

  • adding async support.
  • "real-time" error handling – i.e. if the first plugin fails, howdoi immediately sends a response
  • optional "supports" attribute for Plugins which is a list containing all the sources they support.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm trying to understand PluginStack. Is it the manager of the plugins installed in howdoi?

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, that's the idea. Whenver it receives a request it tries each plugin in turn. If a plugin fails, it moves on to the next plugin, otherwise it returns the result of that plugin. The code above isn't 100% complete.

Copy link

@csr csr Jun 25, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you. I think the user experience is having the user specify the plugin they want to use with a flag along with their query (at least for now, Hajer is working on a model that automatically detects which plugin is the most appropriate). Do we still need a separate plugin stack class? Maybe if we want to retrieve/update the list of installed plugins...

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think that there has to be a list of installed plugins and a way of resolving/calling them somehow. A class to handle this could still be useful, but it would be a bit different to the PluginStack.

@@ -0,0 +1,3 @@
{
"python.pythonPath": "/Users/cesaredecal/workspace/Environments/howdoi/bin/python"

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You probably shouldn't check this in and instead add it to your .gitignore.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you! I forgot

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Np.

@gleitz gleitz force-pushed the master branch 4 times, most recently from 63b79c0 to 0175d81 Compare September 13, 2020 20:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants